Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs(operators): fix @return docs #5447

Merged
merged 3 commits into from
Mar 17, 2021
Merged

docs(operators): fix @return docs #5447

merged 3 commits into from
Mar 17, 2021

Conversation

jakovljevic-mladen
Copy link
Member

With this PR I'd like to fix the most operators' docs that were never fixed since pipeable operators were introduced. The old docs were pointing that operators were returning Observables which is not the case anymore. So I mostly added couple of words saying that operators are returning functions that return an Observable that does something.

I also fixed types that operator functions return, mostly from some Observable type to some OperatorFunction type. The official docs are actually correctly inferring return type, but the sentences were not in compliance with the type (e.g. map's #Returns section).

I also added couple of new @return and @name for some of the operators that were missing them and I modified some @return docs to better describe what returned Observables are doing.

From the src/internal/operators folder, these files were left unchanged:

  • publishBehavior.ts
  • publishReplay.ts
  • concat.ts
  • zipAll.ts

as they lack documentation.

@jakovljevic-mladen
Copy link
Member Author

I'm really sorry, I accidentally closed this PR. I still want it to be reviewed and considered for merging.

@benlesh
Copy link
Member

benlesh commented Sep 20, 2020

I'm so sorry. I love the spirit of this PR, but I'm afraid I didn't notice it in time and there are conflicts because it's quite large.

Also there's bound to be conflicts in #5729 if we merge this, and that has some very important work in it. If you have the time to do this work after #5729 lands, I'd love to see it. Otherwise, I'm so sorry, but I'll have to mark this as blocked.

@jakovljevic-mladen
Copy link
Member Author

jakovljevic-mladen commented Sep 20, 2020 •
edited
Loading

@benlesh Thanks for letting me know. I'd love to work on this one once again after #5729 is merged to master. I'll wait, rebase and ping you once I update this PR. Also, looks like #5729 is gonna remove lots of stuff (if not all) from my other PR: #5462. If you have time to take a look at it, maybe there are some commits that could still land to master after #5729 is merged.

@jakovljevic-mladen jakovljevic-mladen force-pushed the @return_docs branch 2 times, most recently from a1740f9 to be75629 Compare September 27, 2020 22:19
@jakovljevic-mladen
Copy link
Member Author

@benlesh, a friendly ping.
I've seen some other refactoring PRs, do I need to wait for them to be merged or you can take a look at this now?
Also, I'd like to discuss @name usage: some @name lines were removed over time from some of the operators, while so many other still have them. After I rebased master, some @name lines may have been added. I'd like to create another PR solving this issue: either removing @name lines from every operator or either reverting removed @name lines to every operator. What should I do with them?

@benlesh
Copy link
Member

benlesh commented Sep 28, 2020

A lot of that was old from when we were using ES doc. I've fixed a few as I had time but not many. They should be updated to have only what's necessary in them now that our docs site uses the TypeScript information. But you'll want to run it to check.

@jakovljevic-mladen
Copy link
Member Author

I see. Thanks a lot @benlesh, I agree we should only have stuff that is used. I can check it (and possibly remove it) with my other PR that removes ES Docs stuff.

@benlesh benlesh removed the blocked label Oct 2, 2020
@benlesh
Copy link
Member

benlesh commented Oct 2, 2020

Okay, now that things have settled down a bit, it's probably safer to update these if you want to give it a shot. If there's another PR you have that covers this one, I'll let you close this.

@jakovljevic-mladen
Copy link
Member Author

@benlesh, updated. The other PR I mentioned here is not related to this one. I just wanted to let you know that if there are places in this PR where I have re-introduced @name lines in the docs, I did that accidentally when I was resolving conflicts. But, since my other PR is removing all ES Docs related stuff, I could remove all @name lines from every operator in that PR, if you don't mind.

@jakovljevic-mladen
Copy link
Member Author

@benlesh I'm rebasing this third time 😄 I have now included newly added switchScan operator and I have removed types that were usually written in brackets {} after @return statements - I tested and found out that the docs generator have never included what was written there, but they rather took what was declared as a return type of the function - which is really what we need.

I've seen some of the operators having removed @return statements from the docs (which this PR reverted), so please let me know do we need this or I should go and change this PR by removing every @return statement from every operator.

cartant
cartant approved these changes Mar 17, 2021
View reviewed changes
Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Rebased it and made a few minor changes. (It's all docs comments - no code changes - so I'm merging it.)

@cartant cartant merged commit 105bf99 into ReactiveX:master Mar 17, 2021
@jakovljevic-mladen jakovljevic-mladen deleted the @return_docs branch March 17, 2021 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cartant cartant cartant approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jakovljevic-mladen @benlesh @cartant